fix(development-pr-workflow): backtest-change also triggers for NEW monitors/gates - #555
Conversation
…onitors/gates The command only advertised itself for CHANGES to existing data-driven config, so authoring a brand-new gate monitor with a backtest-derived threshold (the INF-335 EGW-Go blue/green gate) didn't trigger it even though the methodology is identical. Add new-monitor/gate trigger language and mention joint rate-AND-floor replay for gate composites.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📚 Documentation Check ✅Verdict: Passed The only plugin touched (development-pr-workflow) had its version bumped from 2.4.0 to 2.5.0 in plugin.json, and the root CLAUDE.md version table was updated to match in the same diff — satisfying the blocking version-bump requirement. No other missing documentation was found. SummaryPR #555 makes three changes, all in
Version bump check (critical): A file under Bump size: Minor (2.4.0→2.5.0) is defensible — the change adds new backtesting guidance (composite-condition handling) to an existing skill, which the repo's own rules class as a "backward-compatible enhancement." Other docs checked: The plugin's own No blocking or informational gaps found. ✨ No Documentation Updates NeededAll documentation appears to be up to date with the code changes. 🤖 Generated by Claude Documentation Validator | Mode: |
|
● Reviewed · against Latest: ✅ approved ← prior: 🚨 changes requested (1) ← 🚨 changes requested (2) Note Approved. Broadens the Iteration history · 3 reviews2026-08-10 17:24 UTC · ✅ approved · 0 findings ·
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Review verdict: COMMENT
👆 The main review comment above is the source of truth for this PR review. It is automatically updated on each review cycle, so always refer to it for the most current feedback.
This formal review submission is for the verdict only.
…kill The base branch converted the backtest-change command into a thin wrapper over a skill, so the trigger wording this branch broadened now belongs on the skill's description — that is what auto-triggers. - Take next's command description (no longer drives triggering) - Skill description: fires for brand-new monitors/gates, not just edits - Replay step: composite gates must be replayed jointly, since per-condition counts overstate what the gate does - Bump development-pr-workflow 2.4.0 -> 2.4.1 (+ root CLAUDE.md table)
Matches the dominant repo convention (40 of 43 SKILL.md files use a single-line description; only 3 used the folded scalar). Addresses review feedback on the mid-sentence line breaks. Text unchanged.
There was a problem hiding this comment.
Caution
The single-line description conversion left two unquoted ': ' sequences in a plain YAML scalar, so the frontmatter failed to parse and the skill would not have registered at all — the inverse of this PR's goal. Reword those to em dashes rather than quoting: no skill in the repo quotes its description, so quoting would trade one outlier for another. Also bump 2.4.1 -> 2.5.0. Step 4 gained a joint-composite-replay requirement, which is a backward-compatible capability, not the wording change a patch bump implies.
|
@request-claude-review Both findings addressed in ddf4dde:
|
|
↻ Reviewing now · view run ↗ This comment will update when the review completes. Findings will appear in the sticky summary above. |
The added trigger scope pushed the description to 1294 chars, past the 1024-char cap in the Agent Skills spec and the only one in the repo over it. validate-plugin.cjs does not check length, so CI would not catch a truncated or rejected description — which would stop the skill auto-triggering, the opposite of this PR's intent. Trimmed to 973 by cutting non-triggering text rather than trigger phrases: five redundant quoted phrases, the 'always report old N vs new M' directive (already step 4 of the body), and the note that the command loads this skill (already stated in the command file).
|
@request-claude-review Description length finding addressed in aaf6fe9 — 973 chars, 51 under the 1024 cap, trigger phrases preserved (trimmed the restated workflow directive and command-loads-skill note instead). |
There was a problem hiding this comment.
Note
✅ Approved — see full review in the sticky comment ↑
|
↻ Reviewing now · view run ↗ This comment will update when the review completes. Findings will appear in the sticky summary above. |
2 participants
Description
backtest-changetrigger language only advertised itself for changes to existing data-driven config ("Before opening a PR for a data-driven change…"), so authoring a brand-new monitor with a backtest-derived threshold never triggered it — even though the methodology is identical. Concrete miss: the INF-335 EGW-Go blue/green gate PR (Uniswap/backend#11307, 2026-07-30) ran the exact backtest-change workflow (21d live-history replay, threshold headroom statement) manually because the trigger language didn't cover new-monitor authoring.next, which converted this command into a thin wrapper overskills/backtest-change/SKILL.md(#553). The change therefore lands on the skill, since that is what auto-triggers; the command keepsnext's wording.Blast radius:
skills/backtest-change/SKILL.md(description + step 4), plus thedevelopment-pr-workflowversion bump and the rootCLAUDE.mdversion table. Minor bump (2.4.0 → 2.5.0): step 4 gains a backward-compatible workflow requirement, not just wording.Testing
yaml.safe_load→ 4 keys (name,description,allowed-tools,model); an earlier single-line conversion left an unquoted:in the value and did not parse — caught in review, fixed by rewording the two:sequences to em dashes rather than adding quotes (0 of 43 skills quote their description)node scripts/validate-plugin.cjs packages/plugins/development-pr-workflowpasses at 2.5.0SKILL.md/commands//agents/frontmatter for parse errors; the 6 that fail are pre-existing onnextand untouched here